-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bugfix/s3 utils 148 mongodb improvements #300
Bugfix/s3 utils 148 mongodb improvements #300
Conversation
williamlardier
commented
Oct 11, 2023
•
edited
Loading
edited
- bump mongodb driver by bumping arsenal version.
- rework logging to display the mongodb reasons along with the error code.
- add periodic logging to better track the progress of the job in case of problems.
- migrate all code (including tests) to the latest mongodb changes (i.e. callbacks are removed and replaced by promises, cursors are not readable anymore by default).
Hello williamlardier,My role is to assist you with the merge of this Status report is not available. |
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command:
Alternatively, the |
d7a99a3
to
4c1926a
Compare
/create_integration_branches |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option
The following options are set: create_integration_branches |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following options are set: create_integration_branches |
|
@francoisferrand : there is no mongodb-related function in other files as far as I can see. Please share example if you saw some. Usage of already migrated functions does not require any change, as callbacks are kept; we only need to change direct use of the MongoDB driver. We also need this change for the current branch if we want to be able to support this parameter for Artesca 1.7, if we cannot bump arsenal, then we need to create (and maintain) an hotfix branch in Arsenal, for s3utils 1.13. |
Indeed, I missed the fact we did not use mongo driver directly, but through arsenal which has been migrated!
It would be much better to merge on 1.14, and switch to s3utils 1.14 on Artesca : this way we keep the option to stick or revert to 1.13, and do not mix concerns. As far as "product" is concerned, it should not matter that we make the change on 1.13 or 1.14 : they will get the same code either way.... but as far as "engineering" (and maintenance) is concerned, this is the safe way. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
utils/S3UtilsMongoClient.js
Outdated
res => { | ||
// Periodically display information about the cursor | ||
// if more than 30s elapsed | ||
if (Date.now() - startCursorDate > 30000) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will call each time the callback is called, once the 30 second liimit has been reached: need to reset startCursorDate
when we add a log ?
if (Date.now() - startCursorDate > 30000) { | |
const currentDate = Date.now(); | |
if (currentDate - startCursorDate >= 30000) { | |
startCursorDate = currentDate; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 8f7df21
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rebased and squashed the commits, see the updated link: https://github.com/scality/s3utils/pull/300/files#diff-3a778b361ddfba6106347ff0cb053bcbb1a84374fa0d7e6e6413e7c6fb6ca5cdR55
862a619
to
8f7df21
Compare
8f7df21
to
abe7c14
Compare
/approve |
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
The following branches will NOT be impacted:
There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request.
If you need this pull request to be removed from the queue, please contact a The following options are set: approve, create_integration_branches |
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue S3UTILS-148. Goodbye williamlardier. |